Skip to content
This repository was archived by the owner on Dec 9, 2021. It is now read-only.

Review from @anorth at 0d7b13a5#2

Open
anorth wants to merge 52 commits intoanorth/emptyfrom
master
Open

Review from @anorth at 0d7b13a5#2
anorth wants to merge 52 commits intoanorth/emptyfrom
master

Conversation

@anorth
Copy link
Copy Markdown
Member

@anorth anorth commented Sep 25, 2019

This PR is to facilitate review of the state of master at 0d7b13a. This is not intended for merge. Instead, apply changes in response to feedback in a new branch from master.

Copy link
Copy Markdown
Member Author

@anorth anorth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eingenito I've reviewed down to chain.yml. I initially started pointing to the spec in too much detail, but we should just wait a couple of weeks before sweating spec details too much.

I think when you push to master, this PR will automatically update. I'll continue review at some point in any case.

Comment thread api/actors.yml Outdated
Comment thread api/actors.yml Outdated
Comment thread api/actors.yml Outdated
Comment thread api/actors.yml
Comment thread api/actors.yml Outdated
Comment thread api/chain.yml
type: string
description: A base64 encoded representation of the signature over the hash of this entire block with the miner's worker key to ensure that it is not tampered with after creation
example: TODO
ExecutedMessage:
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heads up - this might get more complicated. We might move receipts out of the block, which will make this difficult to construct. See filecoin-project/specs#522

We'll have to think about this, but in the meantime it might be sufficient to remove this abstraction in favour of just exposing resources shaped more like the underlying message and receipt.

Comment thread api/chain.yml
id:
type: string
readOnly: true
example: 'zDPWYqFCyaQ9QJHr1qWZ19rFg3YRkcuXREkvpiHRJZhGN5T8SnyJ'
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Show an example with >1 block

Comment thread api/chain.yml
$ref: '#/models/Message'
401:
$ref: 'api.yml#/components/responses/UnauthorizedError'
messages_collection:
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add a GET here which returns the list of messages in a message collection (the id of which is in the header)

Comment thread api/chain.yml
post:
operationId: createMessage
summary: Send a Message
description: This method sends a new Message to an Actor recipient.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll have to be a bit more explicit here about whether the message is already signed, or is to be signed by the node (and if so, with what key), etc, etc. Ok to pick just one answer for now, but it should be clear.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's obviously not impossible for the calling code to have already signed the message, but it's not exactly simple. Maybe it would be appropriate to specify a different content type that takes a base64 encoded binary representation of a message for signed messages and reserve the JSON content type for unsigned messages?

Comment thread api/chain.yml
description: List Tipsets currently known to this node in reverse height order
tags:
- Tipsets
parameters:
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add optional parameters of the tipset from which to start (default the implicit head), and the height at which to stop (default 0).

@eingenito
Copy link
Copy Markdown
Contributor

eingenito commented Sep 27, 2019

@anorth thanks very much for the detailed review. I understand and am incorporating all the feedback. Some of it will take a little time and I didn't want you to think I had missed it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants